-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create Cassandra db schema on session initialization #5922
base: main
Are you sure you want to change the base?
Create Cassandra db schema on session initialization #5922
Conversation
69275fb
to
bcad4c0
Compare
90368b1
to
afc786d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5922 +/- ##
==========================================
- Coverage 96.43% 96.15% -0.29%
==========================================
Files 355 356 +1
Lines 20157 20290 +133
==========================================
+ Hits 19439 19510 +71
- Misses 530 589 +59
- Partials 188 191 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ution for initialize database Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
pkg/cassandra/config/config.go
Outdated
if !c.Schema.CreateSchema { | ||
return nil | ||
} | ||
cluster, err := c.NewCluster() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, all this code effectively repeats NewSession
, the only difference is the keyspace. Can you move it into a helper createSession(keyspace string)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of newSessionPrerequisites
kind of uses a hack to override cluster.Keyspace
which was set in c.NewCluster()
, so that the connection can be established without keyspace.
So, you are suggesting enclosing this override logic in createSession(keyspace string)
?
Or something like I create a copy of the Configuration
with different keyspace to do the work, so that we don't set cluster.keyspace = ""
after creating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make the new function to depend on the config argument, and pass whichever config is needed in each instance, including cloning it to override keyspace
pkg/cassandra/config/config.go
Outdated
@@ -211,6 +273,23 @@ func (c *Configuration) String() string { | |||
} | |||
|
|||
func (c *Configuration) Validate() error { | |||
govalidator.CustomTypeTagMap.Set("cassandraTTLValidation", func(i interface{}, _ interface{}) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need custom functions, can the conditions not be expressed directly via tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we must have custom logic then just write it directly, with proper error messages - right now you are just returning a Boolean, so at best validator can say "property x did not validate".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need custom functions, can the conditions not be expressed directly via tags?
Couldn't find such validations here: https://github.com/asaskevich/govalidator
if we must have custom logic then just write it directly, with proper error messages - right now you are just returning a Boolean, so at best validator can say "property x did not validate".
Fixed it.
if [ "${SKIP_APPLY_SCHEMA}" = "false" ]; then | ||
apply_schema "$schema_version" "$primaryKeyspace" | ||
apply_schema "$schema_version" "$archiveKeyspace" | ||
fi | ||
|
||
if [ "${jaegerVersion}" = "v1" ]; then | ||
STORAGE=cassandra make storage-integration-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this going to distinguish the two runs with SKIP_APPLY_SCHEMA=true/false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the CI workflow is not providing / varying SKIP_APPLY_SCHEMA, so by default it's false and the schema is being pre-created as before, i.e. none of your new code is being CI-tested
- even if the CI workflow was running a matrix with SKIP_APPLY_SCHEMA varied true/false, the actual test is run with the above command that doesn't tell the test to pass any additional arguments to the code, meaning that the
CreateSchema
field in the config will still be false and won't execute your schema creation logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we can add the create: true
in the config-cassandra.yaml, or do we still want to run integration tests with create: false
?
For the workflow we can add the -s
flag in the command based on skip-apply-schema : [true, false]
in the matrix strategy.
Does this sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to have integration tests that do both: (1) script-created schema (as we do today) and (2) in-code created schema (your fix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use similar logic here: https://github.com/jaegertracing/jaeger/blob/58e08c5fcda8d07ac8f7306e40efe01766d117bc/cmd/jaeger/internal/integration/e2e_integration.go#L123C1-L126C3
Let me update it accordingly.
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Alok Kumar Singh <[email protected]>
pkg/cassandra/config/config.go
Outdated
// newSessionPrerequisites creates tables and types before creating a session | ||
func (c *Configuration) newSessionPrerequisites() error { | ||
cfg := *c // clone because we need to connect without specifying a keyspace | ||
cfg.Schema.Keyspace = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make fmt
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
line = line[0:commentIndex] | ||
} | ||
|
||
if len(line) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would make sense to trim spaces before checking for len=0.
if len(queryString) > 0 { | ||
return nil, errors.New(`invalid template`) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do it in generateSchemaIfNotPresent against casQueries
, not here.
return casQueries, nil | ||
} | ||
|
||
func generateSchemaIfNotPresent(session cassandra.Session, cfg *Schema) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are too many functions in this file that are polluting the overall package namespace. I would prefer to introduce a helper struct
type schemaCreator struct {
session cassandra.Session
cfg *Schema
}
and define those functions on that struct (and minimize parameter passing)
Signed-off-by: Alok Kumar Singh <[email protected]>
|
||
c.Schema.Keyspace = "" | ||
|
||
session, err := createSession(c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
session, err := createSession(c) | |
session, err := createSession(cfg) |
return err | ||
} | ||
|
||
return generateSchemaIfNotPresent(session, &c.Schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return generateSchemaIfNotPresent(session, &c.Schema) | |
return generateSchemaIfNotPresent(session, &cfg.Schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this one
Create Schema (if not present) on Session Initialization
Once a session is established with cassandra db, the added code parses the template file containing queries for creating schema and create queries out of it. Post which it executes those queries to create the required types and tables.
Which problem is this PR solving?
Resolves #5797
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test